Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(koa): add layer type to request hook context #1226

Merged

Conversation

unflxw
Copy link
Contributor

@unflxw unflxw commented Oct 11, 2022

Which problem is this PR solving?

Unlike the context given to the Express request hook, the Koa request hook context does not contain information on whether the layer whose instrumentation is being intercepted by the request hook is a middleware layer or a router layer.

Short description of the changes

  • Add the layerType to the context object passed to the requestHook interceptor function.
  • Add the layerType property to the KoaRequestInfo context type.
  • Add a check on the layer type to the unit tests.

@unflxw unflxw requested a review from a team October 11, 2022 10:54
@unflxw unflxw force-pushed the add-layer-type-to-koa-request-hook-context branch from 76bae85 to 2cda169 Compare October 11, 2022 11:32
@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #1226 (ad7604f) into main (180b336) will increase coverage by 2.09%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
+ Coverage   96.08%   98.17%   +2.09%     
==========================================
  Files          14        5       -9     
  Lines         893      164     -729     
  Branches      191       32     -159     
==========================================
- Hits          858      161     -697     
+ Misses         35        3      -32     
Impacted Files Coverage Δ
...ode/opentelemetry-instrumentation-koa/src/types.ts 100.00% <ø> (ø)
...lemetry-instrumentation-koa/src/instrumentation.ts 97.67% <100.00%> (ø)
...metry-propagator-ot-trace/src/OTTracePropagator.ts
...ation-user-interaction/src/enums/AttributeNames.ts
...y-instrumentation-long-task/src/instrumentation.ts
...etry-plugin-react-load/src/enums/AttributeNames.ts
...etapackages/auto-instrumentations-web/src/utils.ts
...strumentation-document-load/src/instrumentation.ts
...erator-aws-xray/src/internal/xray-id-generation.ts
...trumentation-document-load/src/enums/EventNames.ts
... and 11 more

@unflxw
Copy link
Contributor Author

unflxw commented Oct 13, 2022

The failure in the browser tests seems unrelated to me (different package) -- let me know if there's something that needs to be looked at before merging this.

@unflxw unflxw force-pushed the add-layer-type-to-koa-request-hook-context branch from 2cda169 to 4284f87 Compare October 14, 2022 09:05
@blumamir blumamir merged commit 6300733 into open-telemetry:main Oct 18, 2022
@dyladan dyladan mentioned this pull request Oct 18, 2022
@unflxw unflxw deleted the add-layer-type-to-koa-request-hook-context branch November 3, 2022 10:08
tombruijn added a commit to tombruijn/opentelemetry-js-contrib that referenced this pull request Nov 30, 2022
Add the layerType argument to the requestHook function. This is like the
following PR but for restify:
open-telemetry#1226

Move the LayerType from internal-types to types, because it's now used
in a function used by users of the instrumentation package.
blumamir added a commit that referenced this pull request Dec 13, 2022
* feat(restify): add requestHook support

The `requestHook` config option allows custom span handling per request
layer.

* fix(restify): pass config to super class

As mentioned in the review, pass the instrumentation config to the
parent class. That way the config is also stored when given to the
initializer, rather only when using the `setConfig` function.

* fix(restify): fix comment referencing restify type

Update comment to reference to correct type from the `@types/restify`
package.

* fix(restify): import missing Span type

Add the missing import reported by the linter.

* fix(restify): fix issues reported by linter

* fix(restify): fix comment referencing restify type

Mention the package name exactly.

* fix(restify): fix comment referencing restify type

Co-authored-by: Amir Blum <[email protected]>

* fix(restify): add missing import in restify test

* feat(restify): add layer argument to requestHook

Add the layerType argument to the requestHook function. This is like the
following PR but for restify:
#1226

Move the LayerType from internal-types to types, because it's now used
in a function used by users of the instrumentation package.

Co-authored-by: Amir Blum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants